Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename OPAQUE to OPAQUE_ENUM to deconflict with define in wingdi.h #796

Closed
wants to merge 1 commit into from

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Jan 19, 2024

Renamed CesiumGltf::Material::AlphaMode::OPAQUE to OPAQUE_ENUM to deconflict with the define in wingdi.h

The fix was to add OPAQUE to the reserved words list and add a suffix to enums using reserved words.

Now projects using CesiumGltf don't need to add code like this everywhere:

#pragma push_macro("OPAQUE")
#undef OPAQUE

Unfortunately, this is a breaking change and it looks a little odd to see OPAQUE_ENUM, MASK, BLEND. So I'm open to alternatives here.

@csciguy8
Copy link
Contributor

Have you considered using a scoped enum instead? (C++ 11 feature)

struct AlphaMode

becomes

enum struct AlphaMode

This way we can keep our enum names, but also not play the "this platform's scope is polluted with X" game.

@lilleyse
Copy link
Contributor Author

The original reason for using a struct with constants as opposed to an enum is described here: #258 (comment)

I'm not sure if an enum struct would work anyways. Here's a Godbolt example.

@csciguy8
Copy link
Contributor

Good example. I can't get that to work either.

Looks like if OPAQUE is already defined, this will cause problems defining AlphaMode, regardless if it's scoped or not.

I suppose the code that includes wingdi.h can try to stuff it into a separate namespace, but if we are looking for simple, it's hard to beat the renaming approach you did.

@timoore
Copy link
Contributor

timoore commented Jan 22, 2024

Good example. I can't get that to work either.

Looks like if OPAQUE is already defined, this will cause problems defining AlphaMode, regardless if it's scoped or not.

I suppose the code that includes wingdi.h can try to stuff it into a separate namespace, but if we are looking for simple, it's hard to beat the renaming approach you did.

I think that is going to be unwinnable, given that the offending symbols are macros.

I believe that spdlog is sucking wingdi.h into Cesium Native headers by including windows.h somewhere, and it might be possible rearrange or forward-declare the spdlog declarations so that wouldn't happen anymore in Cesium Native clients. But then any user who included windows.h themselves would run into this problem again.

@kring
Copy link
Member

kring commented Jan 22, 2024

Possible related:
#584

I have more to say on this, will type it up soon.

@kring
Copy link
Member

kring commented Jan 22, 2024

In my view, windows.h and friends are a total disaster. Including it in most projects, or alongside many third-party libraries, will cause all kinds of confusing errors. It has macros for all kinds of common things; not just OPAQUE but also MIN and MAX (also min and max I believe). It appends - again via the preprocessor - nonsense like _A and _W to the end of a bunch of common-ish names.

The only winning move, IMO, is not to play. Don't include windows.h. In the rare case that you're writing non-portable code that needs to use windows functionality directly, include windows.h only in a .cpp file that implements functions, declared in a header file (that does not include Windows headers), that can be called by the rest of your application. In other words, isolate the functionality in a single compilation unit that provides an interface to Windows without including any third-party libraries.

#584 describes a case where a third-party library used by cesium-native fails to do this. The easiest workaround in that case is to enable exceptions, but it's probably reasonably easy to fix the library instead if necessary.

Unless there's a case I'm not aware of where this sort of approach creates an undue burden, I'd much prefer it over modifying cesium-native to avoid Windows' many macros.

@lilleyse
Copy link
Contributor Author

lilleyse commented Feb 1, 2024

Something else must be including windows.h in cesium-omniverse because we already have exceptions enabled. @timoore mentioned spdlog. It could also be a library in omniverse. I haven't looked deeper yet.

I'm going to close this PR, but it would still be nice to fix this somehow.

@lilleyse lilleyse closed this Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants